Skip to content

Don't add default Connection header if it exists #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

vkill
Copy link
Contributor

@vkill vkill commented Aug 2, 2019

Hi there,

The NIOSSL.NIOSSLError.uncleanShutdown was happened when I post https://accounts.google.com/o/oauth2/token using the following code, because the Connection: close header was automatic added.

// main.swift

import AsyncHTTPClient
import NIO

let eventLoop = MultiThreadedEventLoopGroup(numberOfThreads: 1).next()
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoop))

try httpClient.post(url: "https://accounts.google.com/o/oauth2/token", body: .string("")).flatMap { response -> EventLoopFuture<()> in
    print(response.status)
    return eventLoop.makeSucceededFuture(())
}.flatMapError { error in
    // NIOSSL.NIOSSLError.uncleanShutdown happened
    print(error)
    return eventLoop.makeFailedFuture(error)
}.wait()

So, I think we should not add it if user provide Connection header. In this case, user can make request by try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, headers: .init([("Connection", "keep-alive")]), body: .string("")).

Please review it, thx.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@Lukasa
Copy link
Collaborator

Lukasa commented Aug 2, 2019

I think I disagree with this patch. Right now async-http-client doesn't support keep-alive connections, so Connection: close is the correct header field to send to tell the server we aren't going to re-use this.

What should actually happen is that async-http-client should swallow the uncleanShutdown error and use its own logic to detect whether a response was truncated or not.

@vkill
Copy link
Contributor Author

vkill commented Aug 2, 2019

Yes, you are right, I will continue resolve it.

@vkill vkill closed this Aug 2, 2019
@vkill vkill reopened this Aug 2, 2019
@vkill vkill closed this Aug 3, 2019
@vkill
Copy link
Contributor Author

vkill commented Aug 5, 2019

@Lukasa This is a google API bug, the Content-Length: n or Transfer-Encoding: chunked did not return in response when I request curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -d '' -v, so the response state does not switch to .end after connection close.

curl examples:

curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 03:48:43 GMT
* Server ESF is not blacklisted
< Server: ESF
< Cache-Control: private
< X-XSS-Protection: 0
< X-Frame-Options: SAMEORIGIN
< X-Content-Type-Options: nosniff
< Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
< Accept-Ranges: none
< Vary: Origin,Accept-Encoding
< Connection: close
curl --http1.1 https://accounts.google.com/o/oauth2/token -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 05:01:48 GMT
* Server ESF is not blacklisted
< Server: ESF
< Cache-Control: private
< X-XSS-Protection: 0
< X-Frame-Options: SAMEORIGIN
< X-Content-Type-Options: nosniff
< Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
< Accept-Ranges: none
< Vary: Origin,Accept-Encoding
< Transfer-Encoding: chunked
curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: keep-alive' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 05:03:34 GMT
* Server ESF is not blacklisted
< Server: ESF
< Cache-Control: private
< X-XSS-Protection: 0
< X-Frame-Options: SAMEORIGIN
< X-Content-Type-Options: nosniff
< Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
< Accept-Ranges: none
< Vary: Origin,Accept-Encoding
< Transfer-Encoding: chunked
curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -H 'Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: Origin
< Vary: X-Origin
< Vary: Referer
< Content-Encoding: gzip
< Date: Mon, 05 Aug 2019 05:06:41 GMT
* Server ESF is not blacklisted
< Server: ESF
< Cache-Control: private
< X-XSS-Protection: 0
< X-Frame-Options: SAMEORIGIN
< X-Content-Type-Options: nosniff
< Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
< Connection: close
< Transfer-Encoding: chunked

@vkill
Copy link
Contributor Author

vkill commented Aug 5, 2019

So in this case, we can make request by try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, headers: .init([("Accept-Encoding", "gzip;q=1.0,deflate;q=0.6,identity;q=0.3")]), body: .string("")), then unzip the response body data by GzipSwift.

@Lukasa
Copy link
Collaborator

Lukasa commented Aug 5, 2019

Huh, that's a thoroughly unfortunate bug on Google's part. Do we have any idea where this should be reported?

@vkill
Copy link
Contributor Author

vkill commented Aug 5, 2019

Do we have any idea where this should be reported?

Do you mean to report it to google? I have reported it on https://issuetracker.google.com/issues .

@vkill
Copy link
Contributor Author

vkill commented Aug 6, 2019

We can also custom delegate to catch NIOSSLError.uncleanShutdown then build Response to compatible with this non-standard response header.

The example code:

import AsyncHTTPClient
import NIO
import NIOSSL
import NIOHTTP1

class MyResponseAccumulator: HTTPClientResponseDelegate {
    public struct Response {
        /// Remote host of the request.
        public var host: String
        /// Response HTTP status.
        public var status: HTTPResponseStatus
        /// Reponse HTTP headers.
        public var headers: HTTPHeaders
        /// Response body.
        public var body: ByteBuffer?
    }
    
    enum State {
        case idle
        case head(HTTPResponseHead)
        case body(HTTPResponseHead, ByteBuffer)
        case end
        case error(Error)
    }
    
    var state = State.idle
    let request: HTTPClient.Request
    
    var head: HTTPResponseHead? = nil
    var body: ByteBuffer? = nil
    
    init(request: HTTPClient.Request) {
        self.request = request
    }
    
    func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
        switch self.state {
        case .idle:
            self.state = .head(head)
        case .head:
            preconditionFailure("head already set")
        case .body:
            preconditionFailure("no head received before body")
        case .end:
            preconditionFailure("request already processed")
        case .error:
            break
        }
        return task.eventLoop.makeSucceededFuture(())
    }
    
    func didReceivePart(task: HTTPClient.Task<Response>, _ part: ByteBuffer) -> EventLoopFuture<Void> {
        switch self.state {
        case .idle:
            preconditionFailure("no head received before body")
        case .head(let head):
            self.state = .body(head, part)
            self.head = head
            self.body = part
        case .body(let head, var body):
            var part = part
            body.writeBuffer(&part)
            self.state = .body(head, body)
            self.body = body
        case .end:
            preconditionFailure("request already processed")
        case .error:
            break
        }
        return task.eventLoop.makeSucceededFuture(())
    }
    
    func didReceiveError(task: HTTPClient.Task<Response>, _ error: Error) {
        self.state = .error(error)
    }
    
    func didFinishRequest(task: HTTPClient.Task<Response>) throws -> Response {
        switch self.state {
        case .idle:
            preconditionFailure("no head received before end")
        case .head(let head):
            return Response(host: self.request.host, status: head.status, headers: head.headers, body: nil)
        case .body(let head, let body):
            return Response(host: self.request.host, status: head.status, headers: head.headers, body: body)
        case .end:
            preconditionFailure("request already processed")
        case .error(let error):
            switch error {
            case NIOSSLError.uncleanShutdown:
                if let head = self.head, (!head.headers.contains(name: "Content-Length") && head.headers["Transfer-Encoding"].first != "chunked") {
                    if let body = self.body {
                        return Response(host: self.request.host, status: head.status, headers: head.headers, body: body)
                    } else {
                        return Response(host: self.request.host, status: head.status, headers: head.headers, body: nil)
                    }
                } else {
                    throw error
                }
            default:
                throw error
            }
        }
    }
}

let eventLoop = MultiThreadedEventLoopGroup(numberOfThreads: 1).next()
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoop))

let request = try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, body: .string(""))

let delegate = MyResponseAccumulator(request: request)
let task = httpClient.execute(request: request, delegate: delegate)
try task.futureResult.flatMap { response in
    return eventLoop.makeSucceededFuture(response)
}.flatMapError { error -> EventLoopFuture<MyResponseAccumulator.Response> in
    do {
        let response = try delegate.didFinishRequest(task: task)
        return eventLoop.makeSucceededFuture(response)
    } catch {
        return eventLoop.makeFailedFuture(error)
    }
}.flatMap { response -> EventLoopFuture<()> in
    print(response.status.code)
    if var responseBody = response.body {
        print(responseBody.readString(length: responseBody.readableBytes) ?? "")
    }
    return eventLoop.makeSucceededFuture(())
}.wait()

The ruby net/http library can compatible with it.

require 'net/http'

uri = URI('https://accounts.google.com/o/oauth2/token')
req = Net::HTTP::Post.new(uri)
req['Connection'] = 'close'
req['Accept-Encoding'] = 'none'

http = Net::HTTP.new(uri.hostname, uri.port)
http.use_ssl = true
http.set_debug_output $stderr

res = http.start() do |http|
  http.request(req)
end

p res.code
p res.body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants